SpatialCanvas displays image along with other layers #18
Conversation
📝 WalkthroughWalkthroughSpatial canvas delegates image rendering to a new VivSpatialViewer (DetailView-driven) when image layers are present; otherwise it renders a simplified DetailView-based SpatialViewerSimple. Types, channel/config plumbing, and image-loading/getVivLayerProps support were added; runtime dependency Changes
Sequence DiagramsequenceDiagram
participant SV as SpatialViewer
participant LD as LayerData (useLayerData)
participant VSV as VivSpatialViewer
participant VIV as Viv (DetailView)
participant DGL as DeckGL
SV->>LD: request deckLayers and vivLayerProps()
LD-->>SV: deckLayers, vivLayerProps (ImageLayerConfig[])
SV->>SV: determine hasImageLayers
alt has image layers
SV->>VSV: render(vivLayerProps, extraLayers, viewState)
VSV->>LD: request loader / channel defaults (if needed)
LD-->>VSV: ImageLoaderData / ImageLayerConfig[]
VSV->>VIV: create DetailView (width/height, loaders)
VSV->>VSV: toVivViewState(), composeLayers()
VSV->>DGL: render composed (Viv image layers + extra + scale bar)
DGL-->>VSV: onViewStateChange / events
VSV->>VSV: sync per-view state
else no image layers
SV->>DGL: render deckLayers via SpatialViewerSimple (DetailView + DeckGL)
DGL-->>SV: events
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/vis/src/SpatialCanvas/renderers/imageRenderer.ts (1)
27-33:⚠️ Potential issue | 🔴 CriticalFix channel selection typing to resolve TypeScript strict mode compilation failure.
The
extractChannelConfigfunction returns a default object with optionalz/c/tproperties (line 69) while the return type signature (line 62) requires them as non-optional. This causes TypeScript compilation to fail at lines 73 and 80 when returningdefaultsordefaults.selections.The downstream code in
useLayerData.tsalready expectsselectionswith optional properties (usesArray<{ z?: number; c?: number; t?: number }>and constructs selections as[{}]with empty objects). The fix should makez,c, andtoptional across all three locations:
- Line 31:
ImageLayerRenderConfig.channels.selections- Line 56:
extractChannelConfiginput parameter- Line 62:
extractChannelConfigreturn typeProposed fix
- selections: { z: number; c: number; t: number }[]; + selections: { z?: number; c?: number; t?: number }[];Apply the same change to line 56 (input parameter) and line 62 (return type).
🤖 Fix all issues with AI agents
In `@packages/vis/src/SpatialCanvas/index.tsx`:
- Around line 42-45: Two different types named ImageLayerConfig are being
re-exported (one from './types' and one from './useLayerData'), causing an
export conflict; update the re-export from './useLayerData' to alias its type to
a distinct name (e.g., ImageLayerConfigFromUseLayerData or
UseLayerImageLayerConfig) so consumers can import both without ambiguity and
update any internal references to the aliased symbol where needed (look for the
export statement that references ImageLayerConfig from './useLayerData' and
change it to an aliased export, then adjust any uses of that specific type).
In `@packages/vis/src/SpatialCanvas/renderers/pointsRenderer.ts`:
- Around line 77-78: Add an optional boolean use3d flag (default false) to
PointsLayerRenderConfig and update the getPosition accessor in pointsRenderer.ts
to return z = d[2]?.[index] when config.use3d is true, otherwise keep z = 0;
ensure PointsLayerRenderConfig's type declaration includes use3d, that callers
default or pass false to preserve current 2D behavior, and that getPosition
reads the flag (signature using {index, target, ...config}) so 3D coordinates
are only used when explicitly enabled.
In `@packages/vis/src/SpatialCanvas/useLayerData.ts`:
- Around line 322-338: The current logic in useLayerData (after
extractChannelConfig) uses sentinel-value checks to decide if user overrides
exist (e.g., checking channelConfig.colors[0][0] !== 255), which incorrectly
overrides legitimate user-provided values that equal those sentinels; update the
assignments for colors, contrastLimits, channelsVisible, and selections to rely
only on presence checks (e.g., channelConfig.colors.length > 0,
channelConfig.contrastLimits.length > 0, channelConfig.channelsVisible.length >
0, channelConfig.selections.length > 0) and otherwise fall back to imageData.*
or the hardcoded defaults, preserving the existing types for colors:
[number,number,number][], contrastLimits: [number,number][], channelsVisible:
boolean[], and selections: Array<{z?:number;c?:number;t?:number}>; keep
extractChannelConfig usage and simply remove the sentinel value comparisons so
legitimate user values that match former sentinels are honored.
In `@packages/vis/src/SpatialCanvas/VivSpatialViewer.tsx`:
- Around line 131-151: fromVivViewState currently assigns vivViewState.zoom
(which can be number | [number, number]) directly to ViewState.zoom (expects
number); update fromVivViewState to normalize vivViewState.zoom by checking its
type and extracting the first element if it's an array, otherwise use the
numeric value, and default to 0 when undefined so the returned ViewState.zoom is
always a number (refer to function fromVivViewState and
VivViewState.vivViewState.zoom).
- Around line 65-69: The DeckGL ref is typed using the value DeckGL (causing CI
failure); remove the unused deckRef field from the VivSpatialViewerState
interface (symbol: VivSpatialViewerState and property: deckRef) and delete any
other unused deckRef usages in this file (mentions around the other occurrences
of deckRef at the later blocks). If you intend to keep a DeckGL ref, replace the
type with the correct DeckGL ref type (DeckGLRef) instead of DeckGL. Ensure all
references to deckRef (state, initializations, and any refs around
VivSpatialViewer) are either removed or updated to use DeckGLRef consistently.
🧹 Nitpick comments (1)
packages/vis/src/SpatialCanvas/VivSpatialViewer.tsx (1)
293-307: Prefer cloning layers instead of mutatinglayer.props.Direct mutation can bypass deck.gl's change detection. Cloning keeps updates explicit and safer. This pattern is already used elsewhere in this file (line 318-321), so adopting it here ensures consistency.
♻️ Suggested refactor
- const vivLayers = Array.isArray(vivLayersResult) && vivLayersResult.length > 0 + let vivLayers = Array.isArray(vivLayersResult) && vivLayersResult.length > 0 ? (Array.isArray(vivLayersResult[0]) ? vivLayersResult[0] : vivLayersResult) as Layer[] : []; // Apply opacity and visibility to image layers if specified if (firstLayerProps.opacity !== undefined || firstLayerProps.visible !== undefined) { - for (const layer of vivLayers) { - if (firstLayerProps.opacity !== undefined && layer.props.opacity !== firstLayerProps.opacity) { - layer.props = { ...layer.props, opacity: firstLayerProps.opacity }; - } - if (firstLayerProps.visible !== undefined && layer.props.visible !== firstLayerProps.visible) { - layer.props = { ...layer.props, visible: firstLayerProps.visible }; - } - } + vivLayers = vivLayers.map((layer) => + layer.clone({ + ...(firstLayerProps.opacity !== undefined ? { opacity: firstLayerProps.opacity } : {}), + ...(firstLayerProps.visible !== undefined ? { visible: firstLayerProps.visible } : {}), + }) + ); }
| export type * from './types'; | ||
| export { useSpatialViewState, useViewStateUrl } from './hooks'; | ||
| export { VivSpatialViewer } from './VivSpatialViewer'; | ||
| export type { ImageLayerConfig } from './useLayerData'; |
There was a problem hiding this comment.
Resolve duplicate ImageLayerConfig export.
Line 42 already re-exports ImageLayerConfig from ./types, and Line 45 re-exports another ImageLayerConfig from ./useLayerData. These are different shapes, which can cause a TS export conflict or ambiguous API for consumers.
🔧 Suggested fix (alias one of the exports)
-export type { ImageLayerConfig } from './useLayerData';
+export type { ImageLayerConfig as VivImageLayerConfig } from './useLayerData';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export type * from './types'; | |
| export { useSpatialViewState, useViewStateUrl } from './hooks'; | |
| export { VivSpatialViewer } from './VivSpatialViewer'; | |
| export type { ImageLayerConfig } from './useLayerData'; | |
| export type * from './types'; | |
| export { useSpatialViewState, useViewStateUrl } from './hooks'; | |
| export { VivSpatialViewer } from './VivSpatialViewer'; | |
| export type { ImageLayerConfig as VivImageLayerConfig } from './useLayerData'; |
🤖 Prompt for AI Agents
In `@packages/vis/src/SpatialCanvas/index.tsx` around lines 42 - 45, Two different
types named ImageLayerConfig are being re-exported (one from './types' and one
from './useLayerData'), causing an export conflict; update the re-export from
'./useLayerData' to alias its type to a distinct name (e.g.,
ImageLayerConfigFromUseLayerData or UseLayerImageLayerConfig) so consumers can
import both without ambiguity and update any internal references to the aliased
symbol where needed (look for the export statement that references
ImageLayerConfig from './useLayerData' and change it to an aliased export, then
adjust any uses of that specific type).
| // Extract channel config (user-provided overrides) | ||
| const channelConfig = extractChannelConfig(config); | ||
|
|
||
| // Use user-provided config if available, otherwise use computed defaults | ||
| const colors: [number, number, number][] = channelConfig.colors.length > 0 && channelConfig.colors[0][0] !== 255 | ||
| ? channelConfig.colors | ||
| : (imageData.colors || [[255, 255, 255] as [number, number, number]]); | ||
| const contrastLimits: [number, number][] = channelConfig.contrastLimits.length > 0 && channelConfig.contrastLimits[0][1] !== 65535 | ||
| ? channelConfig.contrastLimits | ||
| : (imageData.contrastLimits || [[0, 65535] as [number, number]]); | ||
| const channelsVisible: boolean[] = channelConfig.channelsVisible.length > 0 | ||
| ? channelConfig.channelsVisible | ||
| : (imageData.channelsVisible || [true]); | ||
| const selections: Array<{ z?: number; c?: number; t?: number }> = channelConfig.selections.length > 0 | ||
| ? channelConfig.selections | ||
| : (imageData.selections || [{}]); | ||
|
|
There was a problem hiding this comment.
Avoid sentinel-value checks for user overrides.
At Line 326-334, the code infers “user provided” values by comparing against default values. This breaks for legitimate user settings that equal defaults (e.g., color [255, 0, 0] or contrast [0, 65535]), which will be silently overridden.
✅ Suggested fix (use presence checks instead of sentinel values)
- const channelConfig = extractChannelConfig(config);
-
- const colors: [number, number, number][] = channelConfig.colors.length > 0 && channelConfig.colors[0][0] !== 255
- ? channelConfig.colors
- : (imageData.colors || [[255, 255, 255] as [number, number, number]]);
- const contrastLimits: [number, number][] = channelConfig.contrastLimits.length > 0 && channelConfig.contrastLimits[0][1] !== 65535
- ? channelConfig.contrastLimits
- : (imageData.contrastLimits || [[0, 65535] as [number, number]]);
- const channelsVisible: boolean[] = channelConfig.channelsVisible.length > 0
- ? channelConfig.channelsVisible
- : (imageData.channelsVisible || [true]);
- const selections: Array<{ z?: number; c?: number; t?: number }> = channelConfig.selections.length > 0
- ? channelConfig.selections
- : (imageData.selections || [{}]);
+ const overrides = config.channels ?? {};
+ const colors: [number, number, number][] = overrides.colors ?? imageData.colors ?? [[255, 255, 255]];
+ const contrastLimits: [number, number][] = overrides.contrastLimits ?? imageData.contrastLimits ?? [[0, 65535]];
+ const channelsVisible: boolean[] = overrides.channelsVisible ?? imageData.channelsVisible ?? [true];
+ const selections: Array<{ z?: number; c?: number; t?: number }> = overrides.selections ?? imageData.selections ?? [{}];🤖 Prompt for AI Agents
In `@packages/vis/src/SpatialCanvas/useLayerData.ts` around lines 322 - 338, The
current logic in useLayerData (after extractChannelConfig) uses sentinel-value
checks to decide if user overrides exist (e.g., checking
channelConfig.colors[0][0] !== 255), which incorrectly overrides legitimate
user-provided values that equal those sentinels; update the assignments for
colors, contrastLimits, channelsVisible, and selections to rely only on presence
checks (e.g., channelConfig.colors.length > 0,
channelConfig.contrastLimits.length > 0, channelConfig.channelsVisible.length >
0, channelConfig.selections.length > 0) and otherwise fall back to imageData.*
or the hardcoded defaults, preserving the existing types for colors:
[number,number,number][], contrastLimits: [number,number][], channelsVisible:
boolean[], and selections: Array<{z?:number;c?:number;t?:number}>; keep
extractChannelConfig usage and simply remove the sentinel value comparisons so
legitimate user values that match former sentinels are honored.
| function toVivViewState(viewState: ViewState, viewId: string, width: number, height: number): VivViewState { | ||
| const [x, y, z = 0] = viewState.target; | ||
| return { | ||
| id: viewId, | ||
| target: [x, y, z], | ||
| zoom: viewState.zoom, | ||
| // @ts-expect-error - Viv ViewState may have additional properties | ||
| width, | ||
| height, | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Convert Viv ViewState to SpatialCanvas ViewState format | ||
| */ | ||
| function fromVivViewState(vivViewState: VivViewState): ViewState { | ||
| const target = vivViewState.target as [number, number, number]; | ||
| return { | ||
| target: [target[0], target[1]], | ||
| zoom: vivViewState.zoom, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate the file and read it to confirm the current state
find . -name "VivSpatialViewer.tsx" -type fRepository: Taylor-CCB-Group/SpatialData.js
Length of output: 128
🏁 Script executed:
# Search for VivViewState type definition
rg "type VivViewState|interface VivViewState" -A 10Repository: Taylor-CCB-Group/SpatialData.js
Length of output: 1316
🏁 Script executed:
# Search for ViewState type definition
rg "type ViewState|interface ViewState" -A 10Repository: Taylor-CCB-Group/SpatialData.js
Length of output: 854
🏁 Script executed:
# Let's read the VivSpatialViewer.tsx file to see the imports and the actual code
head -50 packages/vis/src/SpatialCanvas/VivSpatialViewer.tsxRepository: Taylor-CCB-Group/SpatialData.js
Length of output: 2032
🏁 Script executed:
# Search for deck.gl imports to understand where these types come from
rg "import.*OrthographicViewState|import.*OrbitViewState" packages/vis/src/SpatialCanvas/VivSpatialViewer.tsx -A 2 -B 2Repository: Taylor-CCB-Group/SpatialData.js
Length of output: 399
🏁 Script executed:
# Let's also check what the actual fromVivViewState function looks like currently
sed -n '131,160p' packages/vis/src/SpatialCanvas/VivSpatialViewer.tsxRepository: Taylor-CCB-Group/SpatialData.js
Length of output: 990
🏁 Script executed:
# Check package.json to see deck.gl version
rg "deck.gl" package.jsonRepository: Taylor-CCB-Group/SpatialData.js
Length of output: 57
🌐 Web query:
deck.gl OrthographicViewState zoom type documentation
💡 Result:
In deck.gl, the viewState for an OrthographicView uses a zoom field with this type:
zoom?: number | [number, number]
Meaning:
- Single number: uniform zoom on both axes.
zoom: 0maps 1 world unit to 1 screen pixel; each +1 doubles scale (1 → 2×, 2 → 4×, etc.). (deck.gl) - Tuple
[zoomX, zoomY]: independent zoom for X and Y axes. (deck.gl)
Citations:
- 1: https://deck.gl/docs/api-reference/core/orthographic-view?utm_source=openai
- 2: https://deck.gl/docs/api-reference/core/orthographic-view?utm_source=openai
🏁 Script executed:
# Check the exact code around line 150 with context to see if there's a ts-expect-error
sed -n '140,155p' packages/vis/src/SpatialCanvas/VivSpatialViewer.tsxRepository: Taylor-CCB-Group/SpatialData.js
Length of output: 499
🏁 Script executed:
# Search for any TypeScript errors or ts-expect-error comments in this file
rg "ts-expect-error|ts-ignore" packages/vis/src/SpatialCanvas/VivSpatialViewer.tsxRepository: Taylor-CCB-Group/SpatialData.js
Length of output: 413
Normalize zoom when converting from Viv view state.
The zoom property from deck.gl's OrthographicViewState can be number | [number, number], but ViewState.zoom expects only a number. The current code at line 150 directly assigns the mismatched type, causing a TypeScript compilation error. Normalize to a single number by extracting the first element if it's an array, and defaulting to 0 if undefined.
Proposed fix
function fromVivViewState(vivViewState: VivViewState): ViewState {
const target = vivViewState.target as [number, number, number];
+ const zoomValue = Array.isArray(vivViewState.zoom)
+ ? vivViewState.zoom[0]
+ : (vivViewState.zoom ?? 0);
return {
target: [target[0], target[1]],
- zoom: vivViewState.zoom,
+ zoom: zoomValue,
};
}🧰 Tools
🪛 GitHub Actions: Test
[error] 150-150: Type 'number | [number, number] | undefined' is not assignable to type 'number'.
🤖 Prompt for AI Agents
In `@packages/vis/src/SpatialCanvas/VivSpatialViewer.tsx` around lines 131 - 151,
fromVivViewState currently assigns vivViewState.zoom (which can be number |
[number, number]) directly to ViewState.zoom (expects number); update
fromVivViewState to normalize vivViewState.zoom by checking its type and
extracting the first element if it's an array, otherwise use the numeric value,
and default to 0 when undefined so the returned ViewState.zoom is always a
number (refer to function fromVivViewState and VivViewState.vivViewState.zoom).
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/vis/src/SpatialCanvas/renderers/pointsRenderer.ts (1)
26-43:⚠️ Potential issue | 🟡 MinorAdd default value and null-coalescing for z-coordinate to prevent invalid positions with 2D data.
When
use3dis true butpointData.datahas only two rows,d[2]?.[index]returnsundefined, resulting in invalid position vectors[x, y, undefined]passed to deck.gl. Defaultuse3dtofalseand apply null-coalescing on the z-coordinate:Suggested fix
- use3d, + use3d = false, ... - getPosition: (_d, {index, target}) => [d[0][index], d[1][index], use3d ? d[2]?.[index] : 0], + getPosition: (_d, {index, target}) => [d[0][index], d[1][index], use3d ? (d[2]?.[index] ?? 0) : 0],packages/vis/src/SpatialCanvas/types.ts (1)
30-38:⚠️ Potential issue | 🟡 MinorRemove redundant
elementKeydeclaration fromImageLayerConfig.
ImageLayerConfiginheritselementKey: stringfromBaseLayerConfig(line 37), making the redeclaration on line 54 redundant and a maintenance liability. All existing usages already provideelementKeywhen creating layers, so this removal is safe internally.🧹 Remove redundant redeclaration
export interface ImageLayerConfig extends BaseLayerConfig { type: 'image'; - elementKey: string; /** Optional: Advanced channel configuration (for full Viv controls) */ channels?: ChannelConfig; }
🧹 Nitpick comments (1)
packages/vis/src/SpatialCanvas/renderers/pointsRenderer.ts (1)
20-24: AlignPointData.datawith the TypedArray intent.
The comment says TypedArray but the type isnumber[][]. If runtime data is typed arrays, the current typing is misleading. Consider widening the type to accept typed arrays.♻️ Suggested type alignment
export interface PointData { shape: number[]; - // this should most definitely be TypedArray... - data: number[][]; + // Accept TypedArrays while keeping index access. + data: ArrayLike<number>[]; }Please confirm the runtime shape (Array vs TypedArray) before updating.
…aking care of input/output order (but not attempting to reconcile units).
…ned z value with use3d in pointsRenderer.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/core/src/schemas/index.ts`:
- Around line 175-183: The getEnumValues function is using the internal
_def.values and an incorrect type for z.ZodEnum; update it to use the public
schema.options property and fix the type signature to a Zod enum tuple (e.g.,
z.ZodEnum<readonly string[]> or z.ZodEnum<[string, ...string[]]>). Specifically,
in getEnumValues replace the cast/access of _def.values with schema.options and
ensure the function signature and types reflect a Zod enum tuple while still
returning new Set<string>(schema.options).
In `@packages/core/src/transformations/transformations.ts`:
- Around line 32-89: The function mapSpatialValuesToXYZ can misinterpret
legitimate values equal to defaultValue as unassigned; change the logic to track
explicit assignment booleans (e.g., xSet, ySet, zSet) instead of comparing
against defaultValue: set the corresponding flag to true whenever you assign
xValue/yValue/zValue inside the first loop (for axis.type === 'space'), use
those flags when deciding whether to skip assignment, and in the fallback loop
only assign to dimensions whose flag is still false (and then set the flag).
This preserves real values equal to defaultValue and prevents incorrect
overwrites from the spatialAxesInOrder fallback.
🧹 Nitpick comments (4)
packages/core/src/schemas/index.ts (1)
185-197: Consider usingas constassertion for stronger typing.The exported constants are typed as
Set<string>, losing the specific unit string literal types. If downstream code needs to check membership with type narrowing, consider exporting the schema'soptionsarray directly or adding type assertions.This is minor since the primary use case is runtime membership checking.
packages/core/src/transformations/transformations.ts (3)
6-16: Address or remove the inline TODO question.Line 11 contains an open question:
"^^ Do we really need to be supporting this? ^^". This should either be resolved before merging or converted to a proper TODO/FIXME with a tracking issue.Would you like me to open an issue to track the decision on whether backward compatibility for axes-less transformations should be maintained?
37-42: Debug warning should be conditional or use structured logging.This
console.warnwill emit on every call with legacy data. Consider guarding it with a debug flag, using a structured logger, or removing it if the backward compatibility path is stable.
191-209: Validation warnings are helpful but could be more actionable.The dimension mismatch warnings are useful for debugging. Consider including guidance on what the user should do (e.g., "verify your input data" or "check axis configuration").
Minor: The warning on line 202 mentions "Assuming affine matrix represents spatial dimensions only" which duplicates across both branches—this could be extracted.
| /** | ||
| * SHOULD contain the field “unit” to specify the physical unit of this dimension. | ||
| * Extract enum values from a zod enum schema as a Set of strings. | ||
| * Accesses the internal _def.values array from zod enum schemas. | ||
| */ | ||
| function getEnumValues(schema: z.ZodEnum<Record<string, string>>): Set<string> { | ||
| // zod enum schemas have an internal _def.values array | ||
| const values = (schema as unknown as { _def: { values: readonly string[] } })._def.values; | ||
| return new Set(values); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🌐 Web query:
Zod enum options property public API documentation
💡 Result:
z.enum(...) (a ZodEnum) exposes an .options property as part of the documented API, which returns the allowed values as a tuple (e.g. ["Salmon", "Tuna", "Trout"]). See the enums documentation showing FishEnum.options. [1], [2]
🏁 Script executed:
# First, check the Zod version and the file in question
cd packages/core && cat package.json | grep -A 2 -B 2 '"zod"'Repository: Taylor-CCB-Group/SpatialData.js
Length of output: 177
🏁 Script executed:
# Look at the actual file to confirm the code snippet
sed -n '175,183p' src/schemas/index.tsRepository: Taylor-CCB-Group/SpatialData.js
Length of output: 138
🏁 Script executed:
# Check if there are other uses of this pattern in the codebase
rg "_def\.values" --type tsRepository: Taylor-CCB-Group/SpatialData.js
Length of output: 395
🏁 Script executed:
# Check if .options is used elsewhere in the codebase
rg "\.options" packages/core/src --type ts -B 2 -A 2Repository: Taylor-CCB-Group/SpatialData.js
Length of output: 396
🏁 Script executed:
# Navigate to the correct directory and read the file
cat -n packages/core/src/schemas/index.ts | sed -n '170,190p'Repository: Taylor-CCB-Group/SpatialData.js
Length of output: 909
🏁 Script executed:
# Check for imports of zod to understand version
rg "import.*from.*['\"]zod" packages/core/src/schemas/index.ts -A 5 -B 5Repository: Taylor-CCB-Group/SpatialData.js
Length of output: 497
🏁 Script executed:
# Check how getEnumValues is used in the codebase
rg "getEnumValues" packages/core --type ts -B 2 -A 2Repository: Taylor-CCB-Group/SpatialData.js
Length of output: 1197
🏁 Script executed:
# Find the definitions of spaceUnitSchema and timeUnitSchema
rg "spaceUnitSchema\s*=" packages/core/src/schemas/index.ts -B 2 -A 5Repository: Taylor-CCB-Group/SpatialData.js
Length of output: 313
🏁 Script executed:
# Find timeUnitSchema definition
rg "timeUnitSchema\s*=" packages/core/src/schemas/index.ts -B 2 -A 5Repository: Taylor-CCB-Group/SpatialData.js
Length of output: 311
Replace _def.values with the public .options property.
The function accesses _def.values, which is an internal implementation detail of Zod that may break on version upgrades. Additionally, the type signature is incorrect—z.ZodEnum uses a tuple type, not Record<string, string>.
Use the public .options property instead:
♻️ Suggested refactor
-/**
- * Extract enum values from a zod enum schema as a Set of strings.
- * Accesses the internal _def.values array from zod enum schemas.
- */
-function getEnumValues(schema: z.ZodEnum<Record<string, string>>): Set<string> {
- // zod enum schemas have an internal _def.values array
- const values = (schema as unknown as { _def: { values: readonly string[] } })._def.values;
- return new Set(values);
-}
+/**
+ * Extract enum values from a zod enum schema as a Set of strings.
+ * Uses the public `.options` property available on zod enum schemas.
+ */
+function getEnumValues<T extends readonly [string, ...string[]]>(schema: z.ZodEnum<T>): Set<T[number]> {
+ return new Set(schema.options);
+}🤖 Prompt for AI Agents
In `@packages/core/src/schemas/index.ts` around lines 175 - 183, The getEnumValues
function is using the internal _def.values and an incorrect type for z.ZodEnum;
update it to use the public schema.options property and fix the type signature
to a Zod enum tuple (e.g., z.ZodEnum<readonly string[]> or z.ZodEnum<[string,
...string[]]>). Specifically, in getEnumValues replace the cast/access of
_def.values with schema.options and ensure the function signature and types
reflect a Zod enum tuple while still returning new Set<string>(schema.options).
| function mapSpatialValuesToXYZ( | ||
| values: number[], | ||
| axes?: Axis[], | ||
| defaultValue = 1 | ||
| ): [number, number, number] { | ||
| if (!axes || axes.length === 0) { | ||
| // No axes specified - use direct mapping (backward compatibility) | ||
| console.warn("legacy data with no input axis specification - not really expecting to get here?") | ||
| const [x = defaultValue, y = defaultValue, z = defaultValue] = values; | ||
| return [x, y, z]; | ||
| } | ||
|
|
||
| // Map values to Matrix4 dimensions based on axis name | ||
| // Matrix4 uses standard x, y, z ordering | ||
| let xValue = defaultValue; | ||
| let yValue = defaultValue; | ||
| let zValue = defaultValue; | ||
|
|
||
| // Track spatial axes in order for fallback mapping | ||
| const spatialAxesInOrder: Array<{ name: string; value: number }> = []; | ||
|
|
||
| for (let i = 0; i < axes.length && i < values.length; i++) { | ||
| const axis = axes[i]; | ||
| if (axis.type === 'space') { | ||
| const axisName = axis.name.toLowerCase(); | ||
| const value = values[i] ?? defaultValue; | ||
|
|
||
| // Map by exact axis name match (most common case: "x", "y", "z") | ||
| if (axisName === 'x' && xValue === defaultValue) { | ||
| xValue = value; | ||
| } else if (axisName === 'y' && yValue === defaultValue) { | ||
| yValue = value; | ||
| } else if (axisName === 'z' && zValue === defaultValue) { | ||
| zValue = value; | ||
| } else { | ||
| // Store for fallback mapping if name doesn't match exactly | ||
| spatialAxesInOrder.push({ name: axisName, value }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Fallback: if we have unmapped spatial axes, map them in order | ||
| // This handles cases where axis names don't match x/y/z exactly | ||
| // but preserves the spatial ordering (first → x, second → y, third → z) | ||
| let fallbackIndex = 0; | ||
| for (const { value } of spatialAxesInOrder) { | ||
| if (fallbackIndex === 0 && xValue === defaultValue) { | ||
| xValue = value; | ||
| } else if (fallbackIndex === 1 && yValue === defaultValue) { | ||
| yValue = value; | ||
| } else if (fallbackIndex === 2 && zValue === defaultValue) { | ||
| zValue = value; | ||
| } | ||
| fallbackIndex++; | ||
| } | ||
|
|
||
| return [xValue, yValue, zValue]; | ||
| } |
There was a problem hiding this comment.
Bug: Axis values equal to defaultValue can be incorrectly overwritten.
The conditions on lines 60-68 check xValue === defaultValue before assigning. If a legitimate axis value happens to equal the default (e.g., scale = [1, 2, 3] where x-scale is actually 1), the value won't be assigned initially, and a later fallback axis could overwrite it.
Consider tracking whether each dimension has been explicitly set instead of comparing against the default value:
🐛 Proposed fix using explicit assignment tracking
// Map values to Matrix4 dimensions based on axis name
// Matrix4 uses standard x, y, z ordering
let xValue = defaultValue;
let yValue = defaultValue;
let zValue = defaultValue;
+ let xSet = false;
+ let ySet = false;
+ let zSet = false;
// Track spatial axes in order for fallback mapping
const spatialAxesInOrder: Array<{ name: string; value: number }> = [];
for (let i = 0; i < axes.length && i < values.length; i++) {
const axis = axes[i];
if (axis.type === 'space') {
const axisName = axis.name.toLowerCase();
const value = values[i] ?? defaultValue;
// Map by exact axis name match (most common case: "x", "y", "z")
- if (axisName === 'x' && xValue === defaultValue) {
+ if (axisName === 'x' && !xSet) {
xValue = value;
- } else if (axisName === 'y' && yValue === defaultValue) {
+ xSet = true;
+ } else if (axisName === 'y' && !ySet) {
yValue = value;
- } else if (axisName === 'z' && zValue === defaultValue) {
+ ySet = true;
+ } else if (axisName === 'z' && !zSet) {
zValue = value;
+ zSet = true;
} else {
// Store for fallback mapping if name doesn't match exactly
spatialAxesInOrder.push({ name: axisName, value });
}
}
}
// Fallback: if we have unmapped spatial axes, map them in order
- let fallbackIndex = 0;
for (const { value } of spatialAxesInOrder) {
- if (fallbackIndex === 0 && xValue === defaultValue) {
+ if (!xSet) {
xValue = value;
- } else if (fallbackIndex === 1 && yValue === defaultValue) {
+ xSet = true;
+ } else if (!ySet) {
yValue = value;
- } else if (fallbackIndex === 2 && zValue === defaultValue) {
+ ySet = true;
+ } else if (!zSet) {
zValue = value;
+ zSet = true;
}
- fallbackIndex++;
}🤖 Prompt for AI Agents
In `@packages/core/src/transformations/transformations.ts` around lines 32 - 89,
The function mapSpatialValuesToXYZ can misinterpret legitimate values equal to
defaultValue as unassigned; change the logic to track explicit assignment
booleans (e.g., xSet, ySet, zSet) instead of comparing against defaultValue: set
the corresponding flag to true whenever you assign xValue/yValue/zValue inside
the first loop (for axis.type === 'space'), use those flags when deciding
whether to skip assignment, and in the fallback loop only assign to dimensions
whose flag is still false (and then set the flag). This preserves real values
equal to defaultValue and prevents incorrect overwrites from the
spatialAxesInOrder fallback.
1 participant
Still need much better control and probably some refactoring.
Summary by CodeRabbit
New Features
Bug Fixes
Chores